Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

toll booth symbol #3005

Merged
merged 1 commit into from
Feb 21, 2018
Merged

toll booth symbol #3005

merged 1 commit into from
Feb 21, 2018

Conversation

sommerluk
Copy link
Collaborator

@sommerluk sommerluk commented Jan 6, 2018

Adds a toll booth symbol. Based on the icon of @MaestroGlanz, but without the right bollard (because at 14×14, with the bollard it looks to me a little bit like a dripping oil can). Some small geometry adjustments.

Resolves #958

To discuss:

  • Zoom level: 16?
  • barrier=toll_booth on areas? JOSM and iD have a preset for both, points and areas. The wiki allows it on points and areas (right box on the wiki page), but at the same time, the wiki description says “The node should be on the highway the toll applies to, in order to support routing decisions.” and doesn’t describe a correct mapping on areas. Actually about 5% of the toll booth objects in the database are areas – most of them have also a building=* tag.
  • Between ¼ and ⅓ of the toll booth objects have a name. Should this be rendered?

screenshot 1
screenshot 3
screenshot 4

@kocio-pl
Copy link
Collaborator

kocio-pl commented Jan 7, 2018

  1. I think z16+ works good.
  2. I think rendering areas would be OK. In the future we might develop similar scheme to public transport platform/stop position, but currently we lack this precision so rendering would pretend to be precise while it's not.
  3. It would be good to take a look at these names, because I have no idea what it could be. If this is mostly the road ref, then it would be just misused, but if that is not the case, I would render it in general.

BTW: This PR lacks the icon itself.

@matkoniecz
Copy link
Contributor

ping @sommerluk

this PR lacks the icon itself.

1 similar comment
@matkoniecz
Copy link
Contributor

ping @sommerluk

this PR lacks the icon itself.

@sommerluk
Copy link
Collaborator Author

Sorry, I’m quite busy currently, but I hope to update this PR with

  • the missing icon
  • area rendering
  • (maybe) ref rendering

soon.

@sommerluk
Copy link
Collaborator Author

Finally, the missing icon is added to this PR.

But area rendering does not work. I had added a query to id: amenity-low-priority-poly in the hope that this would be enough – without making any additional changes to the .mms file because there anyway we use the class amenity-low-priority which works for both, point and poly. But – it does not work! I think this is because we import barriers as line features, not as polygon features. (By the way: That means that all the other barrier queries within id: amenity-low-priority-poly are also useless.) I’ve tried to place the icon with the line geometry (instead of polygon geometry), but that does not seem to be possible.

After taking a look to the name values of toll booth, I think it is reasonable to render them also. Only very few of them are generic (like “Toll booth”) – most of them are real names that are useful.

It might be better to not put this into amenity-low-priority, but into a layer that renders with higher priority than the road names and one-way arrows (otherwise the icon often stays hidden)…

@kocio-pl
Copy link
Collaborator

kocio-pl commented Feb 4, 2018

So for me it's not a low priority POI, do you plan to change the code to select it as a normal amenity?

@sommerluk sommerluk force-pushed the toll01 branch 2 times, most recently from cb7bacd to c508df2 Compare February 5, 2018 05:49
@sommerluk
Copy link
Collaborator Author

Done.

Point-only – no polygon rendering because of the reason explained above.

I still have to add the label…

@kocio-pl
Copy link
Collaborator

kocio-pl commented Feb 5, 2018

Area rendering should be perfectly possible - see this example:

@kocio-pl
Copy link
Collaborator

kocio-pl commented Feb 5, 2018

Using your code, data inspector in Kosmtik selects it from database:

feature barrier_toll_booth
layer area-barriers

so it should be available for styling.

With low priority layers maybe just ::barrier element was needed:

https://github.com/kocio-pl/openstreetmap-carto/blob/76f93dd1ab49e599815462f3a1f6744c8403d182/amenity-points.mss#L1047

but maybe also putting it in the .amenity-low-priority section, not .points for example, as you did here.

Because you wanted to not use low priority some other changes might be needed, but it looks like doable.

@sommerluk
Copy link
Collaborator Author

Well, I suppose your example works because this particular “barrier=toll_booth” object has also a “building” tag. With my old code, it was working also fine if a “building” tag was present, but it didn’t render on areas without the “building” tag (or something other that makes them considered as area).

How does your code behave on an area that has “barrier=toll_booth” as the only tag?

@dieterdreist
Copy link

dieterdreist commented Feb 5, 2018 via email

@kocio-pl
Copy link
Collaborator

kocio-pl commented Feb 5, 2018

is there really „toll“ on this footway? Maybe that’s a ticket office and not a toll booth?

I don't know this place, I used it simply as a test for this code.

How does your code behave on an area that has “barrier=toll_booth” as the only tag?

Do you have an example? Overpass exceed the limits when I try to find it.

But I think building tag has nothing to do with it. If it was a problem with database, Kosmtik wouldn't detect this object at all. Therefore I think this is a problem with styling only.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Feb 5, 2018

I made a test with prepared data (just removed building and name tags from *.osm file export) and now it's not shown by data inspector with any of our code branches. So it looks that having a building tag makes it selectable somehow. But I guess this would be still good in many cases, as the area might be typically just a building.

@dieterdreist
Copy link

dieterdreist commented Feb 5, 2018 via email

@kocio-pl
Copy link
Collaborator

kocio-pl commented Feb 5, 2018

Yes, I'm aware of this. My position is that since tagging scheme is not precise, we don't have to decide, just like with ferry terminal (which can mean a stop position, small port, big port etc.). So for now it's enough to render as many cases as possible and let mappers decide.

@sommerluk
Copy link
Collaborator Author

sommerluk commented Feb 21, 2018

Okay. Sorry for long waiting time, I’m currently really busy…

PR updated. From my side, it’s ready.

No area rendering for the moment. This is because I do not like the idea that the rendering depends of another, not directly related tag (here: building=*) to make it recognize as area. I think it would be confusing for mappers if sometimes, they see an icon for barrier=toll_booth on areas (with building=*) and sometimes not (without building=*). Area rendering would probably need a change in the lua file (and therefore a database reload).

The “name” tag is rendered now.

@kocio-pl kocio-pl merged commit e38db1c into gravitystorm:master Feb 21, 2018
@kocio-pl
Copy link
Collaborator

I suspected that's the case and I was shy to ask. 😄

Thanks for your work! We can rethink areas rendering later, but for now both nodes and labels are rendered, which is a good start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants